Migrate to react on rails auto-registration #649
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSwitches React on Rails to v16-style auto-registration and generated packs: removes manual client bootstrap, adds stores-only registration, imports generated server bundle, updates pack/layouts/CI, adds ror_components convention and a new ReScript comment view, broadens .gitignore for generated artifacts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI/Developer
participant Rails as Rails App
participant ROR as ReactOnRails v16
participant Shaka as Shakapacker
participant Generated as Generated Packs
participant Browser as Browser
CI->>Rails: run `rails react_on_rails:generate_packs`
Rails->>ROR: generate packs (nested entries)
ROR->>Shaka: produce generated bundles
Shaka-->>Generated: write `.../generated/*`
Rails->>ROR: init with auto_load_bundle, components_subdirectory="ror_components"
ROR-->>Rails: auto-register components from `ror_components`
Rails->>Browser: render layout with appended Stimulus/global packs
Browser->>Generated: load packs (stimulus, generated server/client)
Generated->>ROR: auto-registered components discovered
Generated->>Browser: stores-registration.js registers stores via ReactOnRails.registerStore
sequenceDiagram
autonumber
participant Component as RescriptShow.jsx
participant API as Actions.Fetch.fetchComments
participant Cable as ActionCable::CommentsChannel
Note over Component: on mount
Component->>API: fetchComments()
API-->>Component: Ok(data) / Error
alt Ok
Component->>Component: dispatch SetComments(data)
else Error
Component->>Component: dispatch SetFetchError
end
Component->>Cable: subscribe
Cable-->>Component: connected / received(newComment)
Component->>Component: dispatch SetComments([newComment])
Cable-->>Component: disconnected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Gemfile (1)
48-52: Remove stale net-pop overrideRuby is pinned to 3.3.7 and Gemfile.lock shows net-pop coming from the Git override; no explicit require 'net/pop' was found — remove the Git override to avoid masking upstream fixes and supply-chain risk.
File: Gemfile (lines 48-52)
Apply:
-# Needed until Ruby 3.3.4 is released https://github.com/ruby/ruby/pull/11006 -# Related issue: https://github.com/ruby/net-pop/issues/26 -# TODO: When Ruby 3.3.4 is released, upgrade Ruby and remove this line -gem "net-pop", github: "ruby/net-pop"
🧹 Nitpick comments (10)
config/shakapacker.yml (1)
24-24: Typo in comment: Procfile.dev-static.Minor nit; helps future readers.
Apply:
- # This is false since we're running `bin/shakapacker -w` in Procfile.dev-setic + # This is false since we're running `bin/shakapacker -w` in Procfile.dev-static-assetsProcfile.dev-static-assets (1)
5-5: Avoid environment-specific Redis assumption in comments.“Redis is already running system-wide” may not hold for all devs/CI. Consider rewording to instruct enabling the line if needed.
Procfile.dev-prod-assets (1)
5-6: Ensure packs are precompiled before starting rails in prod-assets mode.This Procfile assumes precompiled assets exist. Document the step (e.g., bin/shakapacker -p and any Rails asset precompile) or add a guard in your dev workflow.
client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx (1)
62-67: Guard unsubscribe to avoid runtime error if subscription creation fails.If subscribeConsumer fails or shape changes, scription may be undefined or lack unsubscribe.
Apply this diff:
- React.useEffect((function () { - var scription = subscribeToCommentsChannel(); - return (function () { - scription.unsubscribe(); - }); - }), []); + React.useEffect((function () { + var scription = subscribeToCommentsChannel(); + return (function () { + if (scription && typeof scription.unsubscribe === "function") { + scription.unsubscribe(); + } + }); + }), []);bin/dev (2)
21-28: Improve fallback error handling when react_on_rails/dev isn’t available.If require_relative fails, the script currently crashes with a stack trace.
Apply this diff:
begin require "bundler/setup" require "react_on_rails/dev" rescue LoadError # Fallback for when gem is not yet installed puts "Loading ReactOnRails development tools..." - require_relative "../../lib/react_on_rails/dev" + begin + require_relative "../../lib/react_on_rails/dev" + rescue LoadError => e + warn "Could not load ReactOnRails dev tools. Ensure the gem is installed or lib/react_on_rails/dev exists. #{e.message}" + exit 1 + end end
43-45: Send unknown-argument errors to stderr and exit non‑zero.Minor polish for CLI ergonomics.
Apply this diff:
- puts "Unknown argument: #{ARGV[0]}" - puts "Run 'bin/dev help' for usage information" + warn "Unknown argument: #{ARGV[0]}" + warn "Run 'bin/dev help' for usage information" exit 1client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx (1)
11-19: Remove unused redirect/error scaffolding or wire it up.error and redirectLocation are never set, so the early-return branch is dead code. Either remove it or implement proper setting logic.
- let error; - let redirectLocation; - const { location } = railsContext; + const { location } = railsContext; - - if (error || redirectLocation) { - return { error, redirectLocation }; - }app/views/layouts/stimulus_layout.html.erb (1)
10-11: Stimulus assets appended in head: OK.If any views rendered with this layout use Redux stores, also append stores-registration here or ensure those pages append it explicitly.
<%= append_stylesheet_pack_tag('stimulus-bundle') %> <%= append_javascript_pack_tag('stimulus-bundle') %> + <%# Uncomment if pages under this layout use ReactOnRails stores %> + <%#= append_javascript_pack_tag('stores-registration') %>config/initializers/react_on_rails.rb (2)
5-7: Consider gating server‑side console replay/logging in production.With broader auto‑registration and SSR enabled elsewhere in this initializer, replaying server logs can leak props/PII into app logs. Suggest restricting to dev/test:
config.replay_console = Rails.env.development? || Rails.env.test? config.logging_on_server = Rails.env.development? || Rails.env.test?
5-7: Optional: simplify server bundle setup via generated entrypoint.If you want to avoid maintaining a handwritten
server-bundle.jsthat imports the generated file, enable:config.make_generated_server_bundle_the_entrypoint = trueIf you do this, adjust
config.server_bundle_js_fileto match the generated entrypoint as needed. See config docs. (shakacode.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
.gitignore(1 hunks).ruby-version(1 hunks)Gemfile(1 hunks)Procfile.dev-prod-assets(1 hunks)Procfile.dev-static-assets(1 hunks)app/views/layouts/application.html.erb(2 hunks)app/views/layouts/stimulus_layout.html.erb(2 hunks)bin/dev(1 hunks)client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx(1 hunks)client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx(1 hunks)client/app/bundles/comments/startup/App/ror_components/App.jsx(1 hunks)client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx(1 hunks)client/app/packs/client-bundle.js(0 hunks)client/app/packs/server-bundle.js(1 hunks)client/app/packs/stimulus-bundle.js(1 hunks)client/app/packs/stores-registration.js(1 hunks)config/initializers/react_on_rails.rb(1 hunks)config/shakapacker.yml(1 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- client/app/packs/client-bundle.js
🧰 Additional context used
🧬 Code graph analysis (1)
client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx (2)
client/app/bundles/comments/reducers/commentsReducer.js (1)
action(18-18)client/app/bundles/comments/components/CommentBox/CommentList/CommentList.jsx (1)
CommentList(17-87)
🪛 GitHub Actions: JS CI
Gemfile
[error] 1-1: Command 'bundle install' failed: Bundler exited with code 18 due to Ruby version mismatch. System Ruby is 3.3.4 but Gemfile specifies 3.3.7.
🔇 Additional comments (22)
.ruby-version (1)
1-1: LGTM: version bump to 3.3.7.Ensure CI picks this up (see Gemfile comment for workflow check).
Use the workflow scan script in the Gemfile comment to confirm all jobs use 3.3.7.
client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx (1)
8-10: Approve — import paths updated correctly; railsContext usage unaffected.No logic changes required; NavigationBar, NavigationBarContainer, and constants/paths imports verified.
client/app/bundles/comments/startup/App/ror_components/App.jsx (1)
5-5: Import path fix looks correct with v16 auto‑registration.NonRouterCommentsContainer.jsx exists at client/app/bundles/comments/containers/NonRouterCommentsContainer.jsx; ReactOnRails.getStore('commentsStore') is present in App.jsx (line 12) and NavigationBarApp.jsx (line 24). Explicit .jsx import removes the need to verify webpack extension resolution.
.gitignore (1)
56-58: Over‑broad ignore — narrow '/generated/' to ReactOnRails pack output.Current pattern in .gitignore (File: .gitignore, ~lines 56–58) can hide unrelated generated sources (GraphQL codegen, etc.). Scope it to the React on Rails pack output paths.
-**/generated** +# React on Rails generated packs only +/client/app/**/generated/** +/client/**/packs/**/generated/**Verification scripts returned no output; confirm whether any
generateddirectories exist in the repo before applying this change.config/shakapacker.yml (1)
10-10: nested_entries enabled — confirm manifest keys + server-bundle alignment
- config/shakapacker.yml: nested_entries: true.
- Found packs: client/app/packs/{stimulus-bundle.js, stores-registration.js, server-bundle.js}.
- Layouts reference packs via append_* helpers: app/views/layouts/application.html.erb, app/views/layouts/stimulus_layout.html.erb (use 'stimulus-bundle' and 'stores-registration').
- server-bundle imports ./../generated/server-bundle-generated.js (client/app/packs/server-bundle.js).
- No manifest.json found in the repo — cannot verify whether compiled manifest keys are nested (and therefore differ from the bare names used in the helpers).
Action: verify the compiled manifest.json on the build output (e.g., public/packs/manifest*.json) contains keys matching the helper arguments OR update the view helpers to use the nested manifest keys; also confirm generated/server-bundle-generated.js is emitted and referenced correctly in the manifest.
package.json (1)
81-81: react-on-rails 16 / shakapacker 8.4 — versions & auto-registration verified; confirm Webpack 5 + Node 22yarn.lock shows react-on-rails@16.0.0 and shakapacker@8.4.0; config/initializers/react_on_rails.rb has config.auto_load_bundle = true and build_production_command uses shakapacker.
- Confirm webpack 5 is present in package.json / yarn.lock and Node 22 is specified (package.json "engines", .nvmrc, or .node-version).
client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx (1)
13-15: Path updates OK — confirm client webpack CSS Modules for .module.scssImports and .module.scss usage look fine. serverWebpackConfig.js sets cssLoader.options.modules = { exportOnlyLocals: true }, but I did not find a client-side rule explicitly targeting /.module.(scss|sass|css)/; ensure the client webpack config (config/webpack/clientWebpackConfig.js or config/webpack/environment.js) enables css-loader modules (e.g., modules: { auto: /.module.\w+$/ } or equivalent).
Procfile.dev-prod-assets (1)
1-4: LGTM: clear header and intent.client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx (2)
12-25: Reducer treats any object action as success. Verify only SetComments objects dispatch here.If other object-shaped actions reach this reducer, they’ll be interpreted as CommentsFetched.
51-56: Confirm ActionCable payload shape.received wraps data in an array for SetComments. Ensure downstream expects an array-of-comment objects.
bin/dev (2)
31-41: Command mapping reads clean.Nice, intuitive aliases (prod/static/hmr/help) and clear Procfile routing.
33-35: Procfiles present — no action required.Procfile.dev, Procfile.dev-static-assets, and Procfile.dev-prod-assets are present in the repo.
client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx (2)
1-1: LGTM: doc comment sync with server side.
6-6: Verified: routes path and default export are correct.client/app/bundles/comments/routes/routes.jsx exists and exports a default JSX routes tree; import '../../../routes/routes.jsx' is valid.
import routes from '../../../routes/routes.jsx';Procfile.dev-static-assets (1)
3-4: LGTM — res:dev script verifiedpackage.json defines
res:devasyarn res:clean && rescript build -w.client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx (1)
6-6: Updated routes import path looks right.../../../routes/routes.jsx resolves correctly from ror_components to comments/routes.
client/app/packs/server-bundle.js (1)
1-5: Good separation: server bundle imports stores for SSR, generated code for auto‑registration.Ensure server-bundle.js is not included on the client to avoid duplicate store registrations.
app/views/layouts/stimulus_layout.html.erb (1)
22-23: Confirm append_ pattern with options under Shakapacker 8.4.*Using stylesheet_pack_tag/javascript_pack_tag with only options relies on previously appended packs. Verify tags render as expected in all envs.
app/views/layouts/application.html.erb (2)
23-25: Verify appended packs render and order is correct.Ensure stores-registration loads before components that access stores, and that these helpers output tags as intended across environments.
8-10: Head includes Stimulus and stores — OK.app/views/layouts/application.html.erb and app/views/layouts/stimulus_layout.html.erb include append_javascript_pack_tag('stores-registration') (and 'stimulus-bundle'); package.json contains @hotwired/stimulus — auto‑registration pack appears loaded, no action required.
client/app/packs/stores-registration.js (1)
6-9: Cannot verify store exports — confirm routerCommentsStore and commentsStore are default-exported store-generator functions and call-sites use exact namesrg output only matched reducers under client/app/bundles/comments/reducers and reported files were skipped; confirm the store file paths/exports and that callers use getStore('routerCommentsStore') and getStore('commentsStore').
client/app/packs/stimulus-bundle.js (1)
18-19: Confirmed: client auto-registration bundle is included at runtime.
app/views/layouts/application.html.erb and app/views/layouts/stimulus_layout.html.erb include append_javascript_pack_tag('stimulus-bundle'), so auto‑registered components will be available to react_component helpers.
Procfile.dev-static-assets
Outdated
| # Note, if using React on Rails localization you will need to run | ||
| # `bundle exec rake react_on_rails:locale` before you run bin/shakapacker | ||
| webpack: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w' | ||
| js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w' |
There was a problem hiding this comment.
❓ Verification inconclusive
Don’t swallow locale task failures; prefer shakapacker:clean over rm -rf.
The current chaining makes bin/shakapacker -w run even if react_on_rails:locale fails. Also avoid rm -rf in scripts; use the rake task.
Apply this diff:
-js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
+js: sh -c 'bundle exec rake react_on_rails:locale && (bundle exec rake shakapacker:clean || true) && bin/shakapacker -w'If you must keep rm, limit the “|| true” to that step only:
-js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
+js: sh -c 'bundle exec rake react_on_rails:locale && (rm -rf public/packs/* || true) && bin/shakapacker -w'Don’t swallow locale task failures; make the watcher conditional on the locale task
Procfile.dev-static-assets: line 12 — the current || true placement makes bin/shakapacker -w run unconditionally; apply one of these diffs:
-js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
+js: sh -c 'bundle exec rake react_on_rails:locale && (bundle exec rake shakapacker:clean || true) && bin/shakapacker -w'Or, if keeping rm:
-js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
+js: sh -c 'bundle exec rake react_on_rails:locale && (rm -rf public/packs/* || true) && bin/shakapacker -w'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w' | |
| js: sh -c 'bundle exec rake react_on_rails:locale && (bundle exec rake shakapacker:clean || true) && bin/shakapacker -w' |
| js: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w' | |
| js: sh -c 'bundle exec rake react_on_rails:locale && (rm -rf public/packs/* || true) && bin/shakapacker -w' |
🤖 Prompt for AI Agents
In Procfile.dev-static-assets around line 12, the current command uses `|| true`
in a way that swallows failures from the locale rake task and causes
`bin/shakapacker -w` to run unconditionally; change the command so the watcher
only runs if the locale task succeeds — either remove `|| true` and chain with
`&&` so `bundle exec rake react_on_rails:locale && rm -rf public/packs/* &&
bin/shakapacker -w`, or if you want to ignore errors from `rm` but not from the
rake task, group/sequence so the rake task must succeed and only the `rm` is
tolerated, e.g. run the rake task, then `rm -rf public/packs/* || true`, then
`&& bin/shakapacker -w`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/js_test.yml (1)
47-49: Good addition; ensure task availability and consider upgrading GH Actions versions.
- Step placement is correct and aligns with v16 auto‑generated packs being .gitignored.
- Add a lightweight preflight to surface a clearer failure if the task isn’t available on older branches, and consider bumping Actions to v4.
- - name: Checkout code - uses: actions/checkout@v3 + - name: Checkout code + uses: actions/checkout@v4 - - uses: ruby/setup-ruby@v1 + - uses: ruby/setup-ruby@v1 with: ruby-version: ${{ matrix.ruby }} bundler-cache: true - - name: Use Node.js ${{ matrix.node }} - uses: actions/setup-node@v3 + - name: Use Node.js ${{ matrix.node }} + uses: actions/setup-node@v4 with: node-version: ${{ matrix.node }} cache: yarn + - name: Verify React on Rails generate_packs task exists + run: bundle exec rails -T | grep -q 'react_on_rails:generate_packs' + - name: Generate React on Rails packs run: bundle exec rails react_on_rails:generate_packs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/js_test.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (22.x, 3.3.4)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/lint_test.yml (1)
46-48: Packs generation step looks right; add debug flag and confirm DB independence.
- Append
--tracefor easier CI debugging.- This job has no DB service; confirm the rake task doesn’t touch the DB under
RAILS_ENV=test. If it does, override toRAILS_ENV=developmentfor this step only.- - name: Generate React on Rails packs - run: bundle exec rails react_on_rails:generate_packs + - name: Generate React on Rails packs + run: bundle exec rails react_on_rails:generate_packs --trace +# If the task requires no DB, keep test. If it does, explicitly switch env just for this step: +# env: +# RAILS_ENV: development.github/workflows/rspec_test.yml (1)
76-78: Good placement; consider ordering vs ReScript and add--trace.
- If generated packs import ReScript outputs, run this after
yarn res:build; otherwise current order is fine.- Add
--tracefor better failure diagnostics.- - name: Generate React on Rails packs - run: bundle exec rails react_on_rails:generate_packs + - name: Generate React on Rails packs + run: bundle exec rails react_on_rails:generate_packs --traceOptional reorder:
- - name: Generate React on Rails packs - run: bundle exec rails react_on_rails:generate_packs - - name: Build Rescript components run: yarn res:build + - name: Generate React on Rails packs + run: bundle exec rails react_on_rails:generate_packs --trace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/lint_test.yml(1 hunks).github/workflows/rspec_test.yml(1 hunks)client/app/bundles/comments/startup/serverRegistration.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (3)
client/app/bundles/comments/startup/serverRegistration.jsx (3)
8-9: Remove duplicate local store registration — stores are registered in client/app/packs/stores-registration.js.client/app/packs/stores-registration.js calls ReactOnRails.registerStore (includes routerCommentsStore); remove the local imports and ReactOnRails.registerStore call in client/app/bundles/comments/startup/serverRegistration.jsx (lines 8-9 and 20-23).
Apply:
-import routerCommentsStore from '../store/routerCommentsStore'; -import commentsStore from '../store/commentsStore'; +// Stores registered via packs/stores-registration.js in v16 migration. -ReactOnRails.registerStore({ - routerCommentsStore, - commentsStore, -}); +// No local store registration.
4-7: Drop manual component registration; verify ReactOnRails v16 auto-registration is enabledSanity check returned no matches for other manual registrations — verification inconclusive. If ReactOnRails v16 with auto_load_bundle is enabled and ror_components are auto-registered, remove the manual imports and the ReactOnRails.register block in client/app/bundles/comments/startup/serverRegistration.jsx (lines 4–18).
Apply:
import ReactOnRails from 'react-on-rails'; - -import App from './App/ror_components/App'; -import RouterApp from './RouterApp/ror_components/RouterApp.server'; -import SimpleCommentScreen from '../components/SimpleCommentScreen/ror_components/SimpleCommentScreen'; -import NavigationBarApp from './NavigationBarApp/ror_components/NavigationBarApp'; +// Components auto-registered by ReactOnRails v16 from ror_components. import routerCommentsStore from '../store/routerCommentsStore'; import commentsStore from ../store/commentsStore'; -import Footer from '../components/Footer/ror_components/Footer'; - -ReactOnRails.register({ - App, - RouterApp, - NavigationBarApp, - SimpleCommentScreen, - Footer, -}); +// No manual component registration needed.
5-5: No client bundles reference RouterApp.server — server-only import confirmedclient/app/bundles/comments/startup/serverRegistration.jsx imports RouterApp.server and that file is used only as the server webpack entry (config/webpack/server.js:23). No other references to serverRegistration.jsx or RouterApp.server were found; client counterpart is at client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx.
justin808
changed the title
|
rebase on master to get the rubocop fix |
ihabadham
force-pushed
the
migrate-to-react-on-rails-v16
branch
from
c775eba to
4383625
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
bin/dev (2)
30-41: Dispatcher looks good; consider a ‘dev’ alias for ergonomics.Optional: accept “dev” in addition to “hmr” and nil.
Apply this diff:
-when "hmr", nil +when "hmr", "dev", nil ReactOnRails::Dev::ServerManager.start(:development, "Procfile.dev")
43-46: Send errors to STDERR and show built-in help.Minor UX: use warn and call show_help.
Apply this diff:
- puts "Unknown argument: #{ARGV[0]}" - puts "Run 'bin/dev help' for usage information" - exit 1 + warn "Unknown argument: #{ARGV[0] || '(none)'}" + ReactOnRails::Dev::ServerManager.show_help + exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.github/workflows/js_test.yml(1 hunks).github/workflows/lint_test.yml(1 hunks).github/workflows/rspec_test.yml(1 hunks).gitignore(1 hunks)Gemfile(1 hunks)Procfile.dev-prod-assets(1 hunks)Procfile.dev-static-assets(1 hunks)app/views/layouts/application.html.erb(2 hunks)app/views/layouts/stimulus_layout.html.erb(2 hunks)bin/dev(1 hunks)client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx(1 hunks)client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx(1 hunks)client/app/bundles/comments/startup/App/ror_components/App.jsx(1 hunks)client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx(1 hunks)client/app/bundles/comments/startup/serverRegistration.jsx(1 hunks)client/app/packs/client-bundle.js(0 hunks)client/app/packs/server-bundle.js(1 hunks)client/app/packs/stimulus-bundle.js(1 hunks)client/app/packs/stores-registration.js(1 hunks)config/initializers/react_on_rails.rb(1 hunks)config/shakapacker.yml(1 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- client/app/packs/client-bundle.js
🚧 Files skipped from review as they are similar to previous changes (20)
- client/app/packs/server-bundle.js
- config/initializers/react_on_rails.rb
- client/app/bundles/comments/startup/App/ror_components/App.jsx
- Gemfile
- client/app/packs/stimulus-bundle.js
- client/app/bundles/comments/startup/serverRegistration.jsx
- client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx
- client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx
- client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx
- app/views/layouts/stimulus_layout.html.erb
- package.json
- client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx
- Procfile.dev-static-assets
- config/shakapacker.yml
- Procfile.dev-prod-assets
- .github/workflows/rspec_test.yml
- .gitignore
- .github/workflows/lint_test.yml
- .github/workflows/js_test.yml
- client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx
🧰 Additional context used
🪛 ESLint
client/app/packs/stores-registration.js
[error] 9-9: Insert ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (22.x, 3.3.4)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (4)
app/views/layouts/application.html.erb (2)
8-10: LGTM! Asset loading strategy aligned with auto-registration.The migration from
client-bundletostimulus-bundleand the addition ofstores-registrationpack correctly supports the new auto-registration approach. The use ofappend_*_pack_tagensures proper loading order.
23-24: Verify the generic pack tag implementation.stylesheet_pack_tag and javascript_pack_tag are used without pack names and likely won't load assets. Repository contains only app/assets/config/manifest.js (Sprockets) and no Webpacker/Shakapacker packs/manifest — confirm which asset system is intended and either:
- If using Webpacker/Shakapacker: specify pack names (e.g. stylesheet_pack_tag 'application'; javascript_pack_tag 'application') and ensure the packs and manifest (e.g. public/packs/manifest.json) exist.
- If using Sprockets: replace with stylesheet_link_tag 'application' and javascript_include_tag 'application'.
File: app/views/layouts/application.html.erb (lines 23–24)client/app/packs/stores-registration.js (1)
1-9: Approve — formatting fixed and store imports verifiedBoth ../bundles/comments/store/routerCommentsStore and ../bundles/comments/store/commentsStore exist and export default store factories, so ReactOnRails.registerStore({ routerCommentsStore, commentsStore }) is correct.
bin/dev (1)
4-20: Header docs are clear and accurate.Matches the dispatcher behavior below.
bin/dev
Outdated
| begin | ||
| require "bundler/setup" | ||
| require "react_on_rails/dev" | ||
| rescue LoadError | ||
| # Fallback for when gem is not yet installed | ||
| puts "Loading ReactOnRails development tools..." | ||
| require_relative "../../lib/react_on_rails/dev" | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Fix fallback require path; '../../lib/…' escapes the repo and will fail. Also tighten LoadError handling.
From bin/dev, the project lib is ../lib, not ../../lib. Recommend nested rescue to show a clearer error if both gem and local copy are unavailable.
Apply this diff:
-begin
- require "bundler/setup"
- require "react_on_rails/dev"
-rescue LoadError
- # Fallback for when gem is not yet installed
- puts "Loading ReactOnRails development tools..."
- require_relative "../../lib/react_on_rails/dev"
-end
+begin
+ require "bundler/setup"
+ require "react_on_rails/dev"
+rescue LoadError => e
+ # Fallback for when the gem is not yet installed
+ warn "Loading ReactOnRails development tools... (#{e.message})" if ENV["DEBUG"]
+ begin
+ require_relative "../lib/react_on_rails/dev"
+ rescue LoadError
+ abort "ReactOnRails dev tools not found. Run `bundle install` or add lib/react_on_rails/dev."
+ end
+endRun to verify whether a local fallback exists (expected: it usually won’t in this repo; ensuring the gem is installed is the path forward):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for local ReactOnRails dev tools under lib/react_on_rails..."
if [ -f lib/react_on_rails/dev.rb ] || [ -d lib/react_on_rails/dev ]; then
echo "Found local fallback."
else
echo "No local fallback found. Ensure react_on_rails gem is installed so require 'react_on_rails/dev' succeeds."
fiLength of output: 175
Fix fallback require path; '../../lib/…' escapes the repo and will fail. Also tighten LoadError handling.
bin/dev (lines 21-28) — switch the fallback to ../lib and nest a second rescue that aborts if neither the gem nor a local copy is available. Verification: lib/react_on_rails/dev not found in repo (no local fallback); ensure the gem is installed or add lib/react_on_rails/dev.
-begin
- require "bundler/setup"
- require "react_on_rails/dev"
-rescue LoadError
- # Fallback for when gem is not yet installed
- puts "Loading ReactOnRails development tools..."
- require_relative "../../lib/react_on_rails/dev"
-end
+begin
+ require "bundler/setup"
+ require "react_on_rails/dev"
+rescue LoadError => e
+ # Fallback for when the gem is not yet installed
+ warn "Loading ReactOnRails development tools... (#{e.message})" if ENV["DEBUG"]
+ begin
+ require_relative "../lib/react_on_rails/dev"
+ rescue LoadError
+ abort "ReactOnRails dev tools not found. Run `bundle install` or add lib/react_on_rails/dev."
+ end
+end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| begin | |
| require "bundler/setup" | |
| require "react_on_rails/dev" | |
| rescue LoadError | |
| # Fallback for when gem is not yet installed | |
| puts "Loading ReactOnRails development tools..." | |
| require_relative "../../lib/react_on_rails/dev" | |
| end | |
| begin | |
| require "bundler/setup" | |
| require "react_on_rails/dev" | |
| rescue LoadError => e | |
| # Fallback for when the gem is not yet installed | |
| warn "Loading ReactOnRails development tools... (#{e.message})" if ENV["DEBUG"] | |
| begin | |
| require_relative "../lib/react_on_rails/dev" | |
| rescue LoadError | |
| abort "ReactOnRails dev tools not found. Run `bundle install` or add lib/react_on_rails/dev." | |
| end | |
| end |
🤖 Prompt for AI Agents
In bin/dev around lines 21 to 28 the fallback require uses
"../../lib/react_on_rails/dev" which escapes the repo and will fail; change it
to "../lib/react_on_rails/dev" and tighten LoadError handling by rescuing
LoadError for the initial gem require, then attempting the local require inside
its own begin/rescue and aborting with a clear error message if that also fails
(i.e., print an explanatory message and exit) so the script fails loudly when
neither the gem nor a local copy is available.
52af588 to
631a699
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
bin/dev (1)
12-12: Update comment to match actual command pattern.Line 12 should reflect the actual command pattern shown in the implementation.
Apply this diff:
-- bin/dev static: Uses Procfile.dev-static-assets +# - bin/dev static: Uses Procfile.dev-static-assetsclient/app/packs/stores-registration.js (1)
2-3: Optional: consider using a webpack alias to avoid deep relative paths.If Shakapacker config supports it, alias
client/app(e.g.,@app) to write@app/bundles/comments/store/...and reduce fragile../hops.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.github/workflows/js_test.yml(1 hunks).github/workflows/lint_test.yml(1 hunks).github/workflows/rspec_test.yml(1 hunks).gitignore(1 hunks)Gemfile(1 hunks)Procfile.dev-prod-assets(1 hunks)Procfile.dev-static-assets(1 hunks)app/views/layouts/application.html.erb(2 hunks)app/views/layouts/stimulus_layout.html.erb(2 hunks)bin/dev(2 hunks)client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx(1 hunks)client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx(1 hunks)client/app/bundles/comments/startup/App/ror_components/App.jsx(1 hunks)client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx(1 hunks)client/app/bundles/comments/startup/serverRegistration.jsx(1 hunks)client/app/packs/client-bundle.js(0 hunks)client/app/packs/server-bundle.js(1 hunks)client/app/packs/stimulus-bundle.js(1 hunks)client/app/packs/stores-registration.js(1 hunks)config/initializers/react_on_rails.rb(1 hunks)config/shakapacker.yml(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- client/app/packs/client-bundle.js
🚧 Files skipped from review as they are similar to previous changes (21)
- client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx
- client/app/packs/stimulus-bundle.js
- client/app/bundles/comments/startup/serverRegistration.jsx
- Procfile.dev-static-assets
- config/initializers/react_on_rails.rb
- app/views/layouts/application.html.erb
- client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx
- .github/workflows/rspec_test.yml
- config/shakapacker.yml
- client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx
- client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx
- .github/workflows/js_test.yml
- .github/workflows/lint_test.yml
- Gemfile
- client/app/bundles/comments/startup/App/ror_components/App.jsx
- Procfile.dev-prod-assets
- app/views/layouts/stimulus_layout.html.erb
- .gitignore
- package.json
- client/app/packs/server-bundle.js
- client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx
🧰 Additional context used
🪛 ESLint
client/app/packs/stores-registration.js
[error] 9-9: Insert ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (4)
bin/dev (2)
21-28: Apply the previous fix for incorrect fallback path and poor error handling.This is the same issue flagged in the previous review. The fallback require path
../../lib/react_on_rails/devescapes the repository and will fail. The error handling also needs improvement.Apply this diff:
-begin - require "bundler/setup" - require "react_on_rails/dev" -rescue LoadError - # Fallback for when gem is not yet installed - puts "Loading ReactOnRails development tools..." - require_relative "../../lib/react_on_rails/dev" -end +begin + require "bundler/setup" + require "react_on_rails/dev" +rescue LoadError => e + # Fallback for when the gem is not yet installed + warn "Loading ReactOnRails development tools... (#{e.message})" if ENV["DEBUG"] + begin + require_relative "../lib/react_on_rails/dev" + rescue LoadError + abort "ReactOnRails dev tools not found. Run `bundle install` or add lib/react_on_rails/dev." + end +end
31-46: LGTM! Clean command dispatch pattern with proper error handling.The implementation correctly maps different command arguments to appropriate
ReactOnRails::Dev::ServerManagermethods and provides clear error messages for unknown arguments. The case-based dispatch is well-structured and makes the script's behavior transparent.client/app/packs/stores-registration.js (2)
1-3: No action needed — imports are store generator functions.Both routerCommentsStore.js and commentsStore.js default-export arrow functions (
export default (props, railsContext) => { ... }), so they satisfy ReactOnRails' store-generator requirement.
6-9: Correct ReactOnRails store registration — names match Rails usage. app/controllers/pages_controller.rb registers redux_store("routerCommentsStore", ...) and redux_store("commentsStore") (lines ~27 and ~33); layout includes redux_store_hydration_data.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the application to React on Rails v16 with auto-registration capabilities and updates to Shakapacker v8.4. The migration introduces automatic component registration through ror_components directories, eliminating the need for manual component registration and simplifying the development workflow.
- Components are now auto-registered via dedicated
ror_componentsdirectories instead of manual registration - Bundle structure reorganized with separate stores registration and auto-generated client/server bundles
- Development tooling enhanced with updated Procfiles and CI workflows that generate React packs
Reviewed Changes
Copilot reviewed 24 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updates Sass and Shakapacker versions |
| config/shakapacker.yml | Enables nested entries configuration |
| config/initializers/react_on_rails.rb | Configures auto-registration with components subdirectory |
| client/app/packs/stores-registration.js | New centralized store registration file |
| client/app/packs/stimulus-bundle.js | Removes manual component registration, adds explanatory comments |
| client/app/packs/server-bundle.js | Simplifies to import stores and auto-generated bundle |
| client/app/packs/client-bundle.js | File removed as part of auto-registration migration |
| Multiple ror_components files | Components moved to auto-registration directory structure with updated import paths |
| Layout files | Updates asset loading to use append helpers and deferred loading |
| Procfile variants | Clarifies development modes and processes |
| CI workflow files | Adds React pack generation step before tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -0,0 +1,129 @@ | |||
| // Generated by ReScript, PLEASE EDIT WITH CARE | |||
There was a problem hiding this comment.
This comment indicates this is a generated file, but it's being committed to version control. Generated files should typically be in .gitignore unless they're meant to be manually edited. Consider clarifying whether this file should be version controlled or generated during build.
| // Generated by ReScript, PLEASE EDIT WITH CARE | |
| // This file is generated by ReScript but is intentionally committed to version control and may be manually edited. |
Copilot uses AI. Check for mistakes.
| @@ -1,9 +1,9 @@ | |||
| // Compare to ../ClientRouterApp.jsx | |||
| // Compare to ./RouterApp.client.jsx | |||
There was a problem hiding this comment.
The comment references './RouterApp.client.jsx' but the actual file appears to be located at the same directory level. Verify that this relative path is correct for the new directory structure.
Copilot uses AI. Check for mistakes.
| @@ -1,9 +1,9 @@ | |||
| // Compare to ../ServerRouterApp.jsx | |||
| // Compare to ./RouterApp.server.jsx | |||
There was a problem hiding this comment.
The comment references './RouterApp.server.jsx' but the actual file appears to be located at the same directory level. Verify that this relative path is correct for the new directory structure.
Copilot uses AI. Check for mistakes.
|
/deploy-review-app |
|
✅ Deployment complete for PR #649, commit 84316f4 |
|
✅ Deployment complete for PR #649, commit 76f0fee |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/views/layouts/application.html.erb(1 hunks)app/views/layouts/stimulus_layout.html.erb(1 hunks)pr-review-command.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (22.x, 3.3.4)
- GitHub Check: deploy
- GitHub Check: claude-review
| <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> | ||
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> |
There was a problem hiding this comment.
Pass the pack name into the asset tag helpers. stylesheet_pack_tag and javascript_pack_tag both require at least one pack name argument. Calling them with only keyword options (Lines 12–13) raises ArgumentError, so every page using this layout will fail to render. Please supply the proper pack key (likely 'application', or whichever entrypoint now carries your global assets) before the options hash.
- <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %>
- <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %>
+ <%= stylesheet_pack_tag('application', media: 'all', 'data-turbolinks-track': true) %>
+ <%= javascript_pack_tag('application', 'data-turbolinks-track': true, defer: true) %>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> | |
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> | |
| <%= stylesheet_pack_tag('application', media: 'all', 'data-turbolinks-track': true) %> | |
| <%= javascript_pack_tag('application', 'data-turbolinks-track': true, defer: true) %> |
🤖 Prompt for AI Agents
In app/views/layouts/application.html.erb around lines 12 to 13, the
stylesheet_pack_tag and javascript_pack_tag are being called with only keyword
options which raises ArgumentError; pass the pack name (e.g., "application" or
your actual Webpacker entrypoint) as the first argument to each helper before
the options hash so each call becomes stylesheet_pack_tag('application', media:
'all', 'data-turbolinks-track': true) and javascript_pack_tag('application',
'data-turbolinks-track': true, defer: true).
Code Review: Migration to React on Rails v16 Auto-RegistrationI've reviewed PR #649 and the migration to React on Rails v16 auto-registration with Shakapacker v8.4. Here's my comprehensive feedback: ✅ Strengths
🔍 Areas for Attention1. Asset Loading Order (Performance)The layout files now use
2. Generated Files in Version Control
3. Component Path ChangesMultiple components were moved to
4. Bundle Size ImplicationsThe removal of manual bundle splitting might affect initial load:
🐛 Potential Issues
🔒 Security Considerations
📋 Recommendations
✅ Overall AssessmentThis is a well-executed migration that modernizes the codebase to use React on Rails v16's auto-registration feature. The changes follow best practices and maintain backward compatibility where needed. The PR is ready for merge after addressing the minor concerns above. Approval: ✅ Approved with suggestions The migration aligns with React on Rails v16 best practices and should improve maintainability. Great work on organizing the component structure and maintaining clear separation of concerns! |
|
✅ Deployment complete for PR #649, commit 23b25b3 |
|
✅ Deployment complete for PR #649, commit e6cd9f4 |
Code Review: Migrate to react_on_rails v16 and shakapacker v8.4SummaryThis PR successfully upgrades the application to react_on_rails v16 with auto-registration and shakapacker v8.4, introducing significant architectural improvements including component auto-discovery, centralized store registration, and enhanced development tooling. ✅ Strengths
|
Code Review for PR #649: Migrate to react on rails auto-registrationThank you for this comprehensive PR upgrading to React on Rails v16 with auto-registration! I've reviewed the changes, and here's my feedback: ✅ Strengths
🔍 Observations & Minor Suggestions
|
Code Review for PR #649: React on Rails v16 MigrationThank you for this comprehensive migration to React on Rails v16 with auto-registration! This is a substantial improvement that modernizes the codebase. Here is my detailed review: ✅ Strengths1. Auto-Registration Implementation
2. Build System Improvements
3. CI/CD Updates
|
|
✅ Deployment complete for PR #649, commit 3aec6d1 |
- Move stylesheet_pack_tag and javascript_pack_tag to head element - Prevents Turbo warning about scripts being loaded from body - Scripts are still deferred to maintain performance - Fixes browser console warning about script evaluation on page changes
- Keep pack tags at end of body to allow react_component append calls - Add data-turbo-suppress-warning to body tags to suppress Turbo warning - This fixes CI failures while maintaining Shakapacker requirements - React components can now properly append their JavaScript packs
This reverts commit 23b25b3.
…bo warning" This reverts commit 76f0fee.
- Add TODO file explaining the root cause of Turbo warning - Document attempted solutions and why they failed - Provide potential future approaches to investigate - This is a cosmetic issue that doesn't affect functionality
justin808
force-pushed
the
migrate-to-react-on-rails-v16
branch
from
b0f8cad to
ab31233
Compare
|
✅ Deployment complete for PR #649, commit ab31233 |
Code Review for PR #649: Migrate to React on Rails v16 Auto-RegistrationGreat work on migrating to React on Rails v16 with auto-registration! This is a significant upgrade that modernizes the codebase. Here's my comprehensive review: ✅ Strengths
🎯 Code Quality & Best Practices
|
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 22 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -0,0 +1,129 @@ | |||
| // Generated by ReScript, PLEASE EDIT WITH CARE | |||
There was a problem hiding this comment.
This comment suggests the file is auto-generated and should not be edited, but it's being added to version control in a PR. Auto-generated files should typically be excluded from version control or the warning should be removed if manual editing is expected.
| // Generated by ReScript, PLEASE EDIT WITH CARE |
Copilot uses AI. Check for mistakes.
| <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> | ||
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> |
There was a problem hiding this comment.
The stylesheet_pack_tag and javascript_pack_tag calls are missing the required pack name parameter. These should specify which pack to load, such as stylesheet_pack_tag('stimulus-bundle', media: 'all', 'data-turbolinks-track': true).
| <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> | |
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> | |
| <%= stylesheet_pack_tag('stimulus-bundle', media: 'all', 'data-turbolinks-track': true) %> | |
| <%= javascript_pack_tag('stimulus-bundle', 'data-turbolinks-track': true, defer: true) %> |
Copilot uses AI. Check for mistakes.
| <%= react_component "Footer" %> | ||
|
|
||
| <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> | ||
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> |
There was a problem hiding this comment.
The stylesheet_pack_tag and javascript_pack_tag calls are missing the required pack name parameter. These should specify which pack to load, such as stylesheet_pack_tag('stimulus-bundle', media: 'all', 'data-turbolinks-track': true).
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> | |
| <%= javascript_pack_tag('application', 'data-turbolinks-track': true, defer: true) %> |
Copilot uses AI. Check for mistakes.
- Move append_ tags before csrf_meta_tags to match application layout - Keep append_ helpers since layout uses React components - This should help with the flaky Stimulus tab test
|
✅ Deployment complete for PR #649, commit 4daa2ec |
Code Review for PR #649: Migrate to React on Rails Auto-RegistrationI've reviewed the changes in this PR that migrates to React on Rails v16 with auto-registration. Here's my feedback: ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
client/app/bundles/comments/startup/App/ror_components/App.jsx (1)
5-5: Avoid brittle deep relatives; consider an alias.Recommend adding a webpack/Shakapacker resolve.alias (e.g., @comments -> client/app/bundles/comments) and importing from '@comments/containers/NonRouterCommentsContainer.jsx'.
client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx (2)
11-21: Remove dead redirect/error code (React Router v6)
error/redirectLocationare never set; this legacy pattern predates v6. Simplify by deleting this block or adapt to v6 redirect handling.- let error; - let redirectLocation; - const { location } = railsContext; + const { location } = railsContext; - - // This tell react_on_rails to skip server rendering any HTML. Note, client rendering - // will handle the redirect. What's key is that we don't try to render. - // Critical to return the Object properties to match this { error, redirectLocation } - if (error || redirectLocation) { - return { error, redirectLocation }; - }
22-33: Drop unsupportedcontextprop for v6 StaticRouterReact Router v6’s
StaticRouterdoesn’t use acontextprop. Passing it is misleading. Remove it.- const context = {}; // Important that you don't do this if you are redirecting or have an error. // eslint-disable-next-line react/display-name return function ServerRouter() { return ( <Provider store={store}> - <StaticRouter location={location} context={context}> + <StaticRouter location={location}> {routes} </StaticRouter> </Provider> ); };.github/workflows/js_test.yml (1)
39-43: Avoid redundantbundle install
ruby/setup-ruby@v1withbundler-cache: truealready installs and caches gems. Removebundle installto save CI time.- - name: Install dependencies - run: | - bundle install - yarn install --frozen-lockfile --non-interactive --prefer-offline + - name: Install dependencies + run: | + yarn install --frozen-lockfile --non-interactive --prefer-offlineTODO_TURBO_WARNING.md (2)
27-29: Fix markdownlint: wrap bare URLsReplace bare URLs with markdown links to satisfy MD034.
-- Shakapacker docs: https://github.com/shakacode/shakapacker#view-helper-append_javascript_pack_tag -- Turbo docs: https://turbo.hotwired.dev/handbook/building#working-with-script-elements -- React on Rails v16 docs: https://www.shakacode.com/react-on-rails/docs/ +- Shakapacker docs: [link](https://github.com/shakacode/shakapacker#view-helper-append_javascript_pack_tag) +- Turbo docs: [link](https://turbo.hotwired.dev/handbook/building#working-with-script-elements) +- React on Rails v16 docs: [link](https://www.shakacode.com/react-on-rails/docs/)I can open a small doc PR to apply this.
3-24: Capture the exact warning stringAdd the exact console warning text so it’s searchable and comparable after fixes.
-## Issue -There's a console warning about Turbo being loaded from within the `<body>` element instead of the `<head>`. +## Issue +Console warning (verbatim): +> [Turbo] Script loaded in <body> — Turbo recommends loading scripts in <head> to avoid re‑evaluation on navigation.client/app/packs/stimulus-bundle.js (1)
12-16: Gate tracing by environmentAlways‑on tracing is noisy. Tie it to NODE_ENV; Shakapacker defines this at build time.
-ReactOnRails.setOptions({ - // traceTurbolinks: process.env.TRACE_TURBOLINKS, // eslint-disable-line no-undef - // process.env.TRACE_TURBOLINKS -> error: development is not defined - traceTurbolinks: true, -}); +ReactOnRails.setOptions({ + traceTurbolinks: process.env.NODE_ENV !== 'production', +});Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/js_test.yml(1 hunks).github/workflows/lint_test.yml(1 hunks).github/workflows/rspec_test.yml(1 hunks).gitignore(1 hunks)Procfile.dev(1 hunks)TODO_TURBO_WARNING.md(1 hunks)app/views/layouts/application.html.erb(2 hunks)app/views/layouts/stimulus_layout.html.erb(2 hunks)client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx(1 hunks)client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx(1 hunks)client/app/bundles/comments/startup/App/ror_components/App.jsx(1 hunks)client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx(1 hunks)client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx(1 hunks)client/app/bundles/comments/startup/serverRegistration.jsx(1 hunks)client/app/packs/client-bundle.js(0 hunks)client/app/packs/server-bundle.js(1 hunks)client/app/packs/stimulus-bundle.js(1 hunks)client/app/packs/stores-registration.js(1 hunks)config/initializers/react_on_rails.rb(1 hunks)config/shakapacker.yml(1 hunks)
💤 Files with no reviewable changes (1)
- client/app/packs/client-bundle.js
🚧 Files skipped from review as they are similar to previous changes (6)
- config/initializers/react_on_rails.rb
- config/shakapacker.yml
- client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx
- app/views/layouts/application.html.erb
- .gitignore
- client/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsx
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
TODO_TURBO_WARNING.md
27-27: Bare URL used
(MD034, no-bare-urls)
28-28: Bare URL used
(MD034, no-bare-urls)
29-29: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (13)
Procfile.dev (1)
5-5: LGTM: use bundle exec for Rails serverGood call. This ensures the app boots with the Gemfile’s locked versions.
client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx (1)
13-15: Import path realignment looks correct.Paths match the new ror_components depth and the CSS module location.
app/views/layouts/stimulus_layout.html.erb (2)
8-9: Stimulus assets placement looks good.Ensure a 'stimulus-bundle' pack is generated in CI and production.
22-23: Fix pack helper calls — missing pack names will raise ArgumentError.Both helpers require at least one pack name.
Apply (replace 'application' with your actual global pack name):
- <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> - <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> + <%= stylesheet_pack_tag('application', media: 'all', 'data-turbolinks-track': true) %> + <%= javascript_pack_tag('application', 'data-turbolinks-track': true, defer: true) %>#!/bin/bash # Find pack tag calls missing a string pack name rg -nP "stylesheet_pack_tag\\(\\s*(?![\"'])" app/ -S rg -nP "javascript_pack_tag\\(\\s*(?![\"'])" app/ -S.github/workflows/lint_test.yml (1)
46-48: CI step placement LGTM.Generating packs before linting matches the new auto-registration flow.
client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsx (1)
1-1: Approve: routes import path parity confirmed
Both RouterApp.client.jsx and RouterApp.server.jsx import routes from '../../../routes/routes.jsx'.client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx (2)
9-9: Store name aligns with registration
getStore('routerCommentsStore')matches the key registered in stores-registration.js. Good.If you later rename the store export, keep the registered key stable or update all
getStoreusages.
6-6: Import path is correct:../../../routes/routes.jsxresolves toclient/app/bundles/comments/routes/routes.jsx.client/app/packs/stimulus-bundle.js (1)
18-19: LGTM: manual registrations removedAuto-registration comment is clear and matches v16 flow.
client/app/packs/stores-registration.js (2)
5-9: No changes needed:stores-registrationpack is already included
Thestores-registrationpack is loaded inapp/views/layouts/application.html.erb(line 10), so no further action is required.
1-9: Stores correctly defined as generator functions BothrouterCommentsStoreandcommentsStoreexport arrow functions returning a Redux store, satisfying theReactOnRails.registerStoreAPI..github/workflows/js_test.yml (1)
47-48: Good: generate packs before tests in all workflows
Aligns with React on Rails v16 auto-registration flow and prevents missing-entry failures. Thebundle exec rails react_on_rails:generate_packsstep is present after dependencies in the JS, lint, and RSpec workflows..github/workflows/rspec_test.yml (1)
76-78: Good add; ensure env/diagnostics are explicit.Pack generation before build is correct. To avoid surprises with RAILS_ENV=test and to get useful failure logs, consider running the rake task with an explicit env and trace.
Option A (preferred):
- run: bundle exec rails react_on_rails:generate_packs + run: RAILS_ENV=development bundle exec rake react_on_rails:generate_packs --traceOption B (keep rails):
- run: bundle exec rails react_on_rails:generate_packs + run: RAILS_ENV=development bundle exec rails react_on_rails:generate_packs --tracePlease confirm the task behaves the same under Rails test env if you opt not to change it. Based on learnings.
| import App from './App/ror_components/App'; | ||
| import RouterApp from './RouterApp/ror_components/RouterApp.server'; | ||
| import SimpleCommentScreen from '../components/SimpleCommentScreen/ror_components/SimpleCommentScreen'; | ||
| import NavigationBarApp from './NavigationBarApp/ror_components/NavigationBarApp'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
If using auto-registration, this file should not manually register components/stores.
With v16 generated packs, components under ror_components are auto-registered. Keeping manual ReactOnRails.register and registerStore here can cause duplicate registrations if this file is ever imported. Prefer removing manual registrations (and the store imports) or delete this file entirely. If you need server-only variants (e.g., RouterApp.server), let the generator handle it.
Please run:
If unused, remove this file; if used for a special case, strip store registration and rely on packs/stores-registration.js. Based on learnings.
Also applies to: 10-10
🏁 Script executed:
#!/bin/bash
# 1) Verify whether this file is referenced anywhere
rg -nP 'bundles/comments/startup/serverRegistration\.(jsx|tsx|js|ts)'
# 2) List all store registrations to spot duplicates with packs/stores-registration
rg -nP 'ReactOnRails\.registerStore\s*\(' -n -C2
# 3) List all component registrations to spot duplicates with generated packs
rg -nP 'ReactOnRails\.register\s*\(' -n -C2Length of output: 1644
Remove duplicate store registrations in serverRegistration.jsx
Remove the ReactOnRails.registerStore({ routerCommentsStore, commentsStore }) call (lines 20–22) and the corresponding imports from client/app/bundles/comments/startup/serverRegistration.jsx; those stores are already registered in client/app/packs/stores-registration.js (lines 6–8). Keep this file (referenced in config/webpack/server.js entry) for server-only component registration.
🤖 Prompt for AI Agents
In client/app/bundles/comments/startup/serverRegistration.jsx (remove the
duplicate store imports and registration): delete the imports for
routerCommentsStore and commentsStore and remove the
ReactOnRails.registerStore({ routerCommentsStore, commentsStore }) call (the
duplicate registration around lines 20–22), leaving only the server-only
component registrations in this file; stores remain registered in
client/app/packs/stores-registration.js so do not re-register them here and keep
the file for server-only component registration referenced by
config/webpack/server.js.
| // Import stores registration | ||
| import './stores-registration'; | ||
|
|
||
| import App from '../bundles/comments/startup/App'; | ||
| import RouterApp from '../bundles/comments/startup/ServerRouterApp'; | ||
| import SimpleCommentScreen from '../bundles/comments/components/SimpleCommentScreen/SimpleCommentScreen'; | ||
| import NavigationBarApp from '../bundles/comments/startup/NavigationBarApp'; | ||
| import routerCommentsStore from '../bundles/comments/store/routerCommentsStore'; | ||
| import commentsStore from '../bundles/comments/store/commentsStore'; | ||
| import Footer from '../bundles/comments/components/Footer/Footer'; | ||
| import RescriptShow from '../bundles/comments/rescript/ReScriptShow.bs.js'; | ||
|
|
||
| ReactOnRails.register({ | ||
| App, | ||
| RouterApp, | ||
| NavigationBarApp, | ||
| SimpleCommentScreen, | ||
| Footer, | ||
| RescriptShow, | ||
| }); | ||
|
|
||
| ReactOnRails.registerStore({ | ||
| routerCommentsStore, | ||
| commentsStore, | ||
| }); | ||
| // import statement added by react_on_rails:generate_packs rake task | ||
| import "./../generated/server-bundle-generated.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
Switch to generated server bundle + isolated store registration looks right.
This matches v16 auto-registration flow: generated pack handles component wiring; stores come from a single place.
Quick check to avoid duplicate registrations with legacy files:
Based on learnings.
🏁 Script executed:
#!/bin/bash
# Is serverRegistration still imported anywhere?
rg -nP 'serverRegistration\.(jsx|tsx|js|ts)'
# All ReactOnRails registrations (components and stores)
rg -nP 'ReactOnRails\.register(Store)?\s*\(' -C2
# Ensure only one import of stores-registration in packs
rg -nP "stores-registration" client/app | sed 's/^/found: /'Length of output: 1722
Replace legacy server registration entry
Update the entry in config/webpack/server.js to point to the new server bundle instead of the old serverRegistration.jsx, preventing duplicate store registrations.
In config/webpack/server.js:
- entry: './client/app/bundles/comments/startup/serverRegistration.jsx',
+ entry: './client/app/packs/server-bundle.js',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Import stores registration | |
| import './stores-registration'; | |
| import App from '../bundles/comments/startup/App'; | |
| import RouterApp from '../bundles/comments/startup/ServerRouterApp'; | |
| import SimpleCommentScreen from '../bundles/comments/components/SimpleCommentScreen/SimpleCommentScreen'; | |
| import NavigationBarApp from '../bundles/comments/startup/NavigationBarApp'; | |
| import routerCommentsStore from '../bundles/comments/store/routerCommentsStore'; | |
| import commentsStore from '../bundles/comments/store/commentsStore'; | |
| import Footer from '../bundles/comments/components/Footer/Footer'; | |
| import RescriptShow from '../bundles/comments/rescript/ReScriptShow.bs.js'; | |
| ReactOnRails.register({ | |
| App, | |
| RouterApp, | |
| NavigationBarApp, | |
| SimpleCommentScreen, | |
| Footer, | |
| RescriptShow, | |
| }); | |
| ReactOnRails.registerStore({ | |
| routerCommentsStore, | |
| commentsStore, | |
| }); | |
| // import statement added by react_on_rails:generate_packs rake task | |
| import "./../generated/server-bundle-generated.js"; | |
| // config/webpack/server.js | |
| module.exports = { | |
| // …other settings… | |
| entry: './client/app/packs/server-bundle.js', | |
| // …remaining settings… | |
| }; |
🤖 Prompt for AI Agents
In config/webpack/server.js around the entry definition, the webpack entry still
points to the old serverRegistration.jsx and causes duplicate store
registrations; update the entry to use the new server bundle at
client/app/packs/server-bundle.js (the file shown at
client/app/packs/server-bundle.js lines 1-5) so webpack builds the new server
bundle instead of the legacy registration file, and remove or replace any
references to serverRegistration.jsx to avoid duplicate imports.
| rails: bundle exec rails s -p 3000 | ||
| wp-client: HMR=true RAILS_ENV=development NODE_ENV=development bin/shakapacker-dev-server | ||
| wp-server: bundle exec rake react_on_rails:locale && HMR=true SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch |
There was a problem hiding this comment.
Prevent startup race: generate packs before webpack processes start
With v16 auto-registration, webpack can error if wp-client/wp-server start before packs exist. Prepend generate_packs to both webpack commands (and optionally Rails) and set HOST/PORT defaults for container/devbox access.
-rails: bundle exec rails s -p 3000
+rails: bash -lc 'bundle exec rails react_on_rails:generate_packs || true; bundle exec rails s -p ${PORT:-3000} -b ${HOST:-0.0.0.0}'
-wp-client: HMR=true RAILS_ENV=development NODE_ENV=development bin/shakapacker-dev-server
+wp-client: bash -lc 'bundle exec rails react_on_rails:generate_packs && HMR=true RAILS_ENV=development NODE_ENV=development bin/shakapacker-dev-server'
-wp-server: bundle exec rake react_on_rails:locale && HMR=true SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch
+wp-server: bash -lc 'bundle exec rails react_on_rails:generate_packs && bundle exec rake react_on_rails:locale && HMR=true SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch'Based on learnings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rails: bundle exec rails s -p 3000 | |
| wp-client: HMR=true RAILS_ENV=development NODE_ENV=development bin/shakapacker-dev-server | |
| wp-server: bundle exec rake react_on_rails:locale && HMR=true SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch | |
| rails: bash -lc 'bundle exec rails react_on_rails:generate_packs || true; bundle exec rails s -p ${PORT:-3000} -b ${HOST:-0.0.0.0}' | |
| wp-client: bash -lc 'bundle exec rails react_on_rails:generate_packs && HMR=true RAILS_ENV=development NODE_ENV=development bin/shakapacker-dev-server' | |
| wp-server: bash -lc 'bundle exec rails react_on_rails:generate_packs && bundle exec rake react_on_rails:locale && HMR=true SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch' |
🤖 Prompt for AI Agents
In Procfile.dev around lines 5 to 7, webpack can race with Rails because packs
may not exist yet and HMR servers may not be reachable from containers/devboxes;
fix by prepending a generate_packs step to the wp-client and wp-server commands
(and optionally to the rails command) so packs are created before starting
webpack processes, and set HOST and PORT defaults for container/devbox access
(e.g., HOST=0.0.0.0 and an appropriate PORT such as 3035) by exporting or
prefixing those env vars on the webpack/rails lines so the dev servers bind to
the container interface.
- Revert to using direct pack tags instead of append_ helpers - Keep client-bundle and stimulus-bundle as originally intended - This layout doesn't need the auto-registration append pattern
|
✅ Deployment complete for PR #649, commit 351824b |
Code Review for PR #649: Migrate to react_on_rails v16 with Auto-Registration🎯 SummaryThis PR successfully migrates to react_on_rails v16 and shakapacker v8.4, implementing the new auto-registration pattern. The migration is well-executed with good attention to maintaining functionality while modernizing the setup. ✅ Strengths
|
- Remove reference to non-existent client-bundle - Use append_ helpers for stimulus-bundle - Add final pack tags at end of body for React components - Matches the pattern used in application.html.erb
|
✅ Deployment complete for PR #649, commit 58846b0 |
Code Review for PR #649: Migrate to react on rails auto-registrationThank you for this comprehensive migration to React on Rails v16 with auto-registration and Shakapacker v8.4! This is a significant upgrade that modernizes the build pipeline. Here's my review: ✅ Strengths
🔍 Areas for Improvement
|
|
/deploy-review-app |
|
✅ Deployment complete for PR #649, commit 58846b0 |
|
✅ Review app for PR #649 was successfully deleted |
Migrate to react_on_rails v16 and shakapacker v8.4 with auto-registration and enhanced dev tooling
This change is
Summary by CodeRabbit